Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delayed fork proposals #89

Merged
merged 3 commits into from
Nov 4, 2023
Merged

Delayed fork proposals #89

merged 3 commits into from
Nov 4, 2023

Conversation

josojo
Copy link
Collaborator

@josojo josojo commented Nov 2, 2023

This PR allows to submit delayed/scheduled fork proposals.

I image that each adjudication framework will enforce its own delay and hence, we don't enforce a specific number.

The main concern about this implementation is that different delays might cause overlapping scheduled forks. This implementation copies the forkschedules then just to the new forking managers. This is okay, as different forks should be scheduled to remove different courts.(This could be enforced by outside non-core helper contracts at the submission time of the tx.

@josojo josojo requested a review from edmundedgar November 2, 2023 10:51
@josojo josojo force-pushed the delayedForkProposals branch from 9694cb9 to f15b4cb Compare November 2, 2023 12:14
@@ -80,7 +50,8 @@ contract ForkingManager is IForkingManager, ForkableStructure {
address _parentContract,
address _globalExitRoot,
uint256 _arbitrationFee,
address _chainIdManager
address _chainIdManager,
ForkProposal[] calldata proposals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really need to manage this here. I'm thinking the ForkingManager just refuses to fork if it's already in the preparing-to-fork state, and then we leave it up to the caller (which I imagine as a reality.eth Arbitrator contract or similar that has already put the thing being disputed in a frozen state) whether it wants to wait until the fork completes and retry, or cancel the arbitration process, return the fee and only retry if the user requests a retry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we really wanna have different "preparation times" for the fork, then we need. Imagine, there is a fork scheduled with 1 month "preparation time" and then a "fast forking" schedule is also hit.

I am not 100% we need different time, but I think its the complexity is not too much to include different times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think we want different times, since the whole ecosystem has to fork so it affects everyone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add, you shouldn't be doing fast-forking of the main ledger, because users can't keep up with it. If you want fast-forking you should use an L3 that specifically expects to be doing fast-forking, for example because it's mainly used by bots or traders or some set of users that can handle this happening.

@josojo josojo force-pushed the delayedForkProposals branch from f15b4cb to 3f55141 Compare November 3, 2023 12:03
@josojo josojo requested a review from edmundedgar November 4, 2023 20:28
Copy link
Contributor

@edmundedgar edmundedgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. It should have more stuff exposed in the interface so we can call it but half of that is already in another unmerged branch so let's do that in another PR

@josojo josojo merged commit 576c0fa into main Nov 4, 2023
2 checks passed
@josojo josojo deleted the delayedForkProposals branch December 21, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants